[APM] Use the outcome field to calculate the transaction error rate chart #75528

cauemarcondes · 2020-08-20T07:56:59Z

Pending: Fix API test

elasticmachine · 2020-08-20T07:57:06Z

Pinging @elastic/apm-ui (Team:apm)

felixbarny · 2020-08-20T08:28:57Z

This defines the error rate as failure/(failure + success), right? Have you considered the alternatives failure/(failure + success + unknown) or even failure/all_transactions (failure/(failure + success + unknown + null))?

cauemarcondes · 2020-08-20T08:35:52Z

This defines the error rate as failure/(failure + success), right?

Correct.

Have you considered the alternatives failure/(failure + success + unknown) or even failure/all_transactions (failure/(failure + success + unknown + null))?

No, I have I understood, based on this comment https://github.com/elastic/apm/pull/299/files#r471580617, that we should not count the unknown documents, that's also why I am not considering null values too. Do you think I should count it too failure/all_transactions?

x-pack/plugins/apm/server/lib/transaction_groups/get_error_rate.ts

…e-outcome

felixbarny · 2020-08-20T09:44:08Z

I've clarified the definition of error rate: elastic/apm@362e2a1

It's now defined as failure/(failure + success), just as you have implemented 👍

cauemarcondes · 2020-08-20T12:45:01Z

@elasticmachine merge upstream

cauemarcondes · 2020-08-24T07:54:58Z

@elasticmachine merge upstream

cauemarcondes · 2020-08-24T12:02:49Z

@sqren do you know what's the correct way to change the esArchiver to add the new field (event.outcome) to fix the API test?

sorenlouv

lgtm

sorenlouv · 2020-08-25T10:38:48Z

@cauemarcondes You can generate new esarchive snapshots with the commands described here. You'll need to ingest data into ES first (eg. ./scripts/compose.py start master --no-kibana --with-opbeans-node --with-opbeans-rum)

…ana into apm-error-rate-outcome

cauemarcondes · 2020-08-26T09:30:33Z

retest

…e-outcome

x-pack/test/apm_api_integration/basic/tests/transaction_groups/error_rate.ts

cauemarcondes · 2020-09-03T08:44:52Z

@dgieselaar I've just updated the snapshot here f459501, also created a new test checking for the anomaly.

dgieselaar

I've left a suggestion to avoid a snapshot-esque test, other than that, LGTM.

dgieselaar · 2020-09-03T15:43:30Z

x-pack/test/apm_api_integration/basic/tests/transaction_groups/error_rate.ts

+
+        expect(response.body).to.eql({
+          noHits: false,
+          erroneousTransactionsRate: [


IMHO this is not really helpful. Can we avoid a snapshot test? e.g. we can count the buckets, or compare the avg only, etc.

I agree, we should spent a little more time finding the right things to test instead of blindly testing the full output (I've been very guilty of this myself). Testing the entire output like this makes the test prone to false positives, where it fails when new properties are added, which leads us to blindly updating it when the implementation changes, thus bugs can creep in.

The challenge by NOT doing snapshot testing is that we don't have as good coverage and bugs can creep in...

I tried to think of the important things to verify, while still making it flexible enough to allow a developer to change the implementation (eg. add new properties).

const { erroneousTransactionsRate } = response.body; // we want to assert that the range is correct it("has the correct start date", async () => { expect(_.first(erroneousTransactionsRate).x).to.be("..."); }); it("has the correct end date", async () => { expect(_.last(erroneousTransactionsRate).x).to.be("..."); }); it("has the correct number of buckets", async () => { expect(erroneousTransactionsRate.length).to.be(1337); }); // we want to assert that the timeseries values align with the average calculated in elasticsearch (not sure they are identical so might not be possible but worth a shot) it("has the correct calculation for average", async () => { const average = _.meanBy(erroneousTransactionsRate, (p) => p.y); expect(average).to.be(response.body.average); });

WDYT?

I like what you suggested @sqren, but I feel that in the test scenarios you described above we are not testing what really matters for the API, that is the error rate. The average is not calculated in ES, it also uses lodash mean, so even if we change the way we calculate the error rate these tests will never fail.

I agree that we should avoid snapshot tests, but in the end, we are testing the return of an API and we must find a balance between these two approaches.

sorenlouv · 2020-09-04T12:16:18Z

x-pack/test/apm_api_integration/basic/tests/transaction_groups/error_rate.ts

+
+        it('has the correct number of buckets', async () => {
+          expect(errorRateResponse.erroneousTransactionsRate.length).to.be(61);
+        });


It think the way we calculated the average was somewhat useful in verifying the error rate values. Without that we should at least verify the first error rate value:

it('has the correct error rate', async () => { expect(first(errorRateResponse.erroneousTransactionsRate)?.y).to.be(...); });

cauemarcondes · 2020-09-07T07:07:13Z

@elasticmachine merge upstream

kibanamachine · 2020-09-07T08:53:41Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 73cd059

Build metrics

async chunks size

id	value	diff	baseline
apm	4.9MB	+34.0B	4.9MB

distributable file count

id	value	diff	baseline
default	45410	+1	45409

History

💚 Build #72600 succeeded e9b9642
💚 Build #72176 succeeded f459501
💔 Build #70472 failed de9b914
💔 Build #70457 failed 817f47a
💔 Build #70449 failed 817f47a

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

…hart (elastic#75528) * replacing error rate to use event.outcome field * addressing PR comment * fixing api test * fixing API test * fixing api tests * rmeoving snapshot from api test * testing error rate Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

…' of github.com:jloleysens/kibana into ilm/fix/pre-existing-policy-with-no-existing-repository * 'ilm/fix/pre-existing-policy-with-no-existing-repository' of github.com:jloleysens/kibana: fix empty string in selected indices (elastic#76855) [Security Solution] Refactor OverviewHost and OverviewNetwork to use Search Strategy (elastic#76409) Use Search API in Timelion (sync) (elastic#75115) [telemetry] expose getIsOptedIn function in plugin start contract (elastic#75143) [ILM] Clean up remaining js files and any typings (elastic#76803) [Logs UI] Shared `<LogStream />` component (elastic#76262) [Security Solution] Add unit test for all hosts (elastic#76752) [Security Solution] Add unit test for authentications search strategy (elastic#76665) Do not apply search source data for tsvb (elastic#75137) [Security Solution] Refactor NetworkDns to use Search Strategy (elastic#76250) [SECURITY SOLUTION] Adds 'cypress:open-as-ci' command (elastic#76125) [Logs UI] Update alert executor tests (elastic#75764) [Functional] Unskip vega tests and fix flakiness (elastic#76600) [Data] Query String Input accepts classname prop (elastic#76848) [ML] Swim lane pagination for viewing by job id (elastic#76847) [Security Solution] Refactor MatrixHistogram to use Search Strategy (elastic#76603) [APM] Use the outcome field to calculate the transaction error rate chart (elastic#75528) [APM] Use observer.hostname instead of observer.name (elastic#76074) Legacy logging: fix remoteAddress being duplicated in userAgent field (elastic#76751)

…s-for-710 * 'master' of github.com:elastic/kibana: [Search] Use async es client endpoints (elastic#76872) fix empty string in selected indices (elastic#76855) [Security Solution] Refactor OverviewHost and OverviewNetwork to use Search Strategy (elastic#76409) Use Search API in Timelion (sync) (elastic#75115) [telemetry] expose getIsOptedIn function in plugin start contract (elastic#75143) [ILM] Clean up remaining js files and any typings (elastic#76803) [Logs UI] Shared `<LogStream />` component (elastic#76262) [Security Solution] Add unit test for all hosts (elastic#76752) [Security Solution] Add unit test for authentications search strategy (elastic#76665) Do not apply search source data for tsvb (elastic#75137) [Security Solution] Refactor NetworkDns to use Search Strategy (elastic#76250) [SECURITY SOLUTION] Adds 'cypress:open-as-ci' command (elastic#76125) [Logs UI] Update alert executor tests (elastic#75764) [Functional] Unskip vega tests and fix flakiness (elastic#76600) [Data] Query String Input accepts classname prop (elastic#76848) [ML] Swim lane pagination for viewing by job id (elastic#76847) [Security Solution] Refactor MatrixHistogram to use Search Strategy (elastic#76603) [APM] Use the outcome field to calculate the transaction error rate chart (elastic#75528) [APM] Use observer.hostname instead of observer.name (elastic#76074) Legacy logging: fix remoteAddress being duplicated in userAgent field (elastic#76751) # Conflicts: # x-pack/plugins/index_lifecycle_management/__jest__/components/edit_policy.test.tsx # x-pack/plugins/index_lifecycle_management/common/types/index.ts # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/node_allocation.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/edit_policy.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/phases/warm_phase.tsx # x-pack/plugins/index_lifecycle_management/public/application/services/api.ts

kibanamachine · 2020-09-09T09:28:45Z

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

kibanamachine · 2020-09-10T09:38:44Z

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

…hart (elastic#75528) * replacing error rate to use event.outcome field * addressing PR comment * fixing api test * fixing API test * fixing api tests * rmeoving snapshot from api test * testing error rate Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> # Conflicts: # x-pack/plugins/apm/common/__snapshots__/elasticsearch_fieldnames.test.ts.snap

kibanamachine · 2020-09-11T09:48:44Z

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

…hart (elastic#75528) * replacing error rate to use event.outcome field * addressing PR comment * fixing api test * fixing API test * fixing api tests * rmeoving snapshot from api test * testing error rate Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> # Conflicts: # x-pack/plugins/apm/common/__snapshots__/elasticsearch_fieldnames.test.ts.snap

…rate chart (#75528) (#77288) * [APM] Use the outcome field to calculate the transaction error rate chart (#75528) * replacing error rate to use event.outcome field * addressing PR comment * fixing api test * fixing API test * fixing api tests * rmeoving snapshot from api test * testing error rate Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> # Conflicts: # x-pack/plugins/apm/common/__snapshots__/elasticsearch_fieldnames.test.ts.snap * removing service map test

dgieselaar · 2020-10-12T14:18:59Z

Looks good in all browsers 👍

replacing error rate to use event.outcome field

d39b494

cauemarcondes added release_note:enhancement v7.10.0 labels Aug 20, 2020

cauemarcondes requested a review from a team as a code owner August 20, 2020 07:56

botelastic bot added the Team:APM All issues that need APM UI Team support label Aug 20, 2020

cauemarcondes requested a review from felixbarny August 20, 2020 07:57

cauemarcondes changed the title ~~apm-error-rate-outcome~~ [APM] Use the outcome field to calculate the transaction error rate chart Aug 20, 2020

dgieselaar reviewed Aug 20, 2020

View reviewed changes

x-pack/plugins/apm/server/lib/transaction_groups/get_error_rate.ts Outdated Show resolved Hide resolved

Merge branch 'master' of github.com:elastic/kibana into apm-error-rat…

4897424

…e-outcome

addressing PR comment

1be58f1

cauemarcondes requested a review from dgieselaar August 20, 2020 09:51

Merge branch 'master' into apm-error-rate-outcome

6db831a

Merge branch 'master' into apm-error-rate-outcome

e565152

cauemarcondes requested a review from sorenlouv August 24, 2020 12:02

dgieselaar approved these changes Aug 25, 2020

View reviewed changes

sorenlouv approved these changes Aug 25, 2020

View reviewed changes

cauemarcondes added 3 commits August 25, 2020 16:38

fixing api test

1399d86

Merge branch 'apm-error-rate-outcome' of github.com:cauemarcondes/kib…

e67cac5

…ana into apm-error-rate-outcome

fixing API test

817f47a

Merge branch 'master' of github.com:elastic/kibana into apm-error-rat…

de9b914

…e-outcome

sorenlouv reviewed Aug 26, 2020

View reviewed changes

x-pack/test/apm_api_integration/basic/tests/transaction_groups/error_rate.ts Outdated Show resolved Hide resolved

dgieselaar approved these changes Sep 3, 2020

View reviewed changes

rmeoving snapshot from api test

2d73328

sorenlouv reviewed Sep 4, 2020

View reviewed changes

testing error rate

e9b9642

Merge branch 'master' into apm-error-rate-outcome

73cd059

cauemarcondes merged commit 7f323a1 into elastic:master Sep 7, 2020

cauemarcondes deleted the apm-error-rate-outcome branch September 7, 2020 09:23

cauemarcondes mentioned this pull request Sep 7, 2020

[7.x] [APM] Use the outcome field to calculate the transaction error rate chart (#75528) #76854

Closed

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Sep 9, 2020

This was referenced Sep 10, 2020

Removing ml-state index from archive #77143

Merged

[7.x] [APM] Use the outcome field to calculate the transaction error rate chart (#75528) | Removing ml-state index from archive (#77143) #77260

Closed

cauemarcondes mentioned this pull request Sep 13, 2020

[7.x] [APM] Use the outcome field to calculate the transaction error rate chart (#75528) #77288

Merged

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Sep 13, 2020

sorenlouv added the apm:test-plan-7.10.0 label Oct 7, 2020

dgieselaar self-assigned this Oct 12, 2020

dgieselaar added the apm:test-plan-done Pull request that was successfully tested during the test plan label Oct 12, 2020

dgieselaar removed their assignment Oct 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[APM] Use the outcome field to calculate the transaction error rate chart #75528

[APM] Use the outcome field to calculate the transaction error rate chart #75528

cauemarcondes commented Aug 20, 2020 •

edited

Loading

elasticmachine commented Aug 20, 2020

felixbarny commented Aug 20, 2020 •

edited

Loading

cauemarcondes commented Aug 20, 2020 •

edited

Loading

felixbarny commented Aug 20, 2020

cauemarcondes commented Aug 20, 2020

cauemarcondes commented Aug 24, 2020

cauemarcondes commented Aug 24, 2020

sorenlouv left a comment

sorenlouv commented Aug 25, 2020

cauemarcondes commented Aug 26, 2020

cauemarcondes commented Sep 3, 2020

dgieselaar left a comment

dgieselaar Sep 3, 2020

sorenlouv Sep 3, 2020 •

edited

Loading

cauemarcondes Sep 4, 2020

sorenlouv Sep 4, 2020 •

edited

Loading

cauemarcondes commented Sep 7, 2020

kibanamachine commented Sep 7, 2020

kibanamachine commented Sep 9, 2020

kibanamachine commented Sep 10, 2020

kibanamachine commented Sep 11, 2020

dgieselaar commented Oct 12, 2020

[APM] Use the outcome field to calculate the transaction error rate chart #75528

[APM] Use the outcome field to calculate the transaction error rate chart #75528

Conversation

cauemarcondes commented Aug 20, 2020 • edited Loading

elasticmachine commented Aug 20, 2020

felixbarny commented Aug 20, 2020 • edited Loading

cauemarcondes commented Aug 20, 2020 • edited Loading

felixbarny commented Aug 20, 2020

cauemarcondes commented Aug 20, 2020

cauemarcondes commented Aug 24, 2020

cauemarcondes commented Aug 24, 2020

sorenlouv left a comment

Choose a reason for hiding this comment

sorenlouv commented Aug 25, 2020

cauemarcondes commented Aug 26, 2020

cauemarcondes commented Sep 3, 2020

dgieselaar left a comment

Choose a reason for hiding this comment

dgieselaar Sep 3, 2020

Choose a reason for hiding this comment

sorenlouv Sep 3, 2020 • edited Loading

Choose a reason for hiding this comment

cauemarcondes Sep 4, 2020

Choose a reason for hiding this comment

sorenlouv Sep 4, 2020 • edited Loading

Choose a reason for hiding this comment

cauemarcondes commented Sep 7, 2020

kibanamachine commented Sep 7, 2020

💚 Build Succeeded

Build metrics

async chunks size

distributable file count

History

kibanamachine commented Sep 9, 2020

kibanamachine commented Sep 10, 2020

kibanamachine commented Sep 11, 2020

dgieselaar commented Oct 12, 2020

cauemarcondes commented Aug 20, 2020 •

edited

Loading

felixbarny commented Aug 20, 2020 •

edited

Loading

cauemarcondes commented Aug 20, 2020 •

edited

Loading

sorenlouv Sep 3, 2020 •

edited

Loading

sorenlouv Sep 4, 2020 •

edited

Loading